fix(s3tables): use 's3' as the default scheme#2313
Conversation
|
Looks like Java uses s3: as the default https://github.com/apache/iceberg/blob/main/aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIO.java |
dannycjones
left a comment
dannycjones
left a comment
There was a problem hiding this comment.
I believe this is the right change to make. It aligns with the Java catalog and with pre-0.9 behavior.
I'd like to try some testing of the following before a +1 here:
- Trying to create a table or write to a table using 0.9
- Then trying to read that table from this change.
I want to be more confident that we don't break anyone on 0.9.
dannycjones
left a comment
dannycjones
left a comment
There was a problem hiding this comment.
LGTM, tested this catalog (#2315) to verify it doesn't work at the moment so there shouldn't be anyone impacted upgrading from 0.9.0.
|
Thanks @dannycjones and @xanderbailey for checking this. |
CTTY
left a comment
CTTY
left a comment
There was a problem hiding this comment.
Hey @rchowell , thanks for the fix! while your fix makes a good sense, I think a cleaner approach is to just remove configured_scheme. Because storage now accepts fully-qualified locations, we can just extract schemes from the input paths. the configured_scheme seems unneeded to me. also see #2245
wdyt?
Sounds great, I'll update in the morning. I have been OOO. Thank you for looking at this. |
…n/loading plus reading/writing (#2315) ## Which issue does this PR close? - Extends #2313 with a test. In particular, I've written a test that verifies creating a table and writing to it, and then loading that table from the catalog and reading from it using the default storage backend. ## What changes are included in this PR? This adds a test that currently fails and will be passing once #2313 is merged. ## Are these changes tested? This change is only a new test. Signed-off-by: Daniel Carl Jones <djonesoa@amazon.com>
6 participants
Which issue does this PR close?
What changes are included in this PR?
Changes the s3tables default scheme to "s3://"
Are these changes tested?
With this change, I can read files resolved through an s3table catalog now.